-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-2858 Use OP_MSG to authenticate if server supports OP_MSG #843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Is this ready for review? |
pymongo/pool.py
Outdated
@@ -552,7 +552,8 @@ def unpin(self): | |||
def hello_cmd(self): | |||
# Handshake spec requires us to use OP_MSG+hello command for the | |||
# initial handshake in load balanced or versioned api mode. | |||
if self.opts.server_api or self.hello_ok or self.opts.load_balanced: | |||
if (self.opts.server_api or self.hello_ok or self.opts.load_balanced | |||
or self.max_wire_version >= 6): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change isn't needed. We already set op_msg_enabled to true when max_wire_version >= 6)
elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yeah. You're correct. This was due to me confusing DRIVERS-1916 with saying that OP_MSG necessitates "hello" not the other way around. Reverted.
test/mockupdb/test_handshake.py
Outdated
error_response = OpReply( | ||
primary_response = {"setName": 'rs', "hosts": hosts, | ||
"minWireVersion": 2, "maxWireVersion": 13} | ||
error_response = OpMsgReply( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is already passing so do we need to change it at all?
test/mockupdb/test_handshake.py
Outdated
"saslSupportedMechs": [ | ||
"SCRAM-SHA-1"]})) | ||
else: | ||
return request.reply(OpMsgReply(**primary_response)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it responds to an OP_QUERY ismaster handshake with an OP_MSG response. Let's change it to use OP_REPLY. Eg: request.reply(**primary_response)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/mockupdb/test_handshake.py
Outdated
client = MongoClient(server.uri, | ||
username='username', | ||
password='password', | ||
appname='my app', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appname is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/mockupdb/test_handshake.py
Outdated
self.addCleanup(client.close) | ||
self.assertRaises(OperationFailure, client.db.collection.find_one, | ||
{"a": 1}) | ||
assert self.found_auth_msg, """Could not find authentication command |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use self.assertTrue with regular string (not triple quotes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you schedule the mockupdb test suite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM
No description provided.